- 
                Notifications
    
You must be signed in to change notification settings  - Fork 35
 
          Implement Demangle and IsClassPolymorphic APIs
          #451
        
          New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
  
    Implement Demangle and IsClassPolymorphic APIs
  
  #451
              Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
        
          
                lib/Interpreter/CppInterOp.cpp
              
                Outdated
          
        
      | std::string DemangleSymbol(const std::string& mangled_name) { | ||
| char* demangle = itaniumDemangle(mangled_name); | ||
| std::string res(demangle); | ||
| std::free(demangle); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: calling legacy resource function without passing a 'gsl::owner<>' [cppcoreguidelines-owning-memory]
    std::free(demangle);
    ^There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just assign to std::string
        
          
                lib/Interpreter/CppInterOp.cpp
              
                Outdated
          
        
      | std::string DemangleSymbol(const std::string& mangled_name) { | ||
| char* demangle = itaniumDemangle(mangled_name); | ||
| std::string res(demangle); | ||
| std::free(demangle); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not manage memory manually; use RAII [cppcoreguidelines-no-malloc]
    std::free(demangle);
    ^        
          
                lib/Interpreter/CppInterOp.cpp
              
                Outdated
          
        
      | std::string DemangleSymbol(const std::string& mangled_name) { | ||
| char* demangle = itaniumDemangle(mangled_name); | ||
| std::string res(demangle); | ||
| std::free(demangle); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "std::free" is directly included [misc-include-cleaner]
lib/Interpreter/CppInterOp.cpp:26:
- #if CLANG_VERSION_MAJOR >= 19
+ #include <cstdlib>
+ #if CLANG_VERSION_MAJOR >= 19| using namespace llvm; | ||
| using namespace clang; | ||
| 
               | 
          ||
| TEST(ScopeReflectionTest, DemangleSymbol) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'TEST' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
TEST(ScopeReflectionTest, DemangleSymbol) {
^9969e0b    to
    9b24c54      
    Compare
  
    
          Codecov ReportAll modified and coverable lines are covered by tests ✅ 
 Additional details and impacted files@@            Coverage Diff             @@
##             main     #451      +/-   ##
==========================================
+ Coverage   70.96%   71.04%   +0.07%     
==========================================
  Files           9        9              
  Lines        3541     3550       +9     
==========================================
+ Hits         2513     2522       +9     
  Misses       1028     1028              
 
  | 
    
8bb8cbf    to
    e6ff45b      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
| #include "clang/AST/DeclBase.h" | ||
| #include "clang/AST/GlobalDecl.h" | ||
| 
               | 
          ||
| #include "gtest/gtest.h" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 'gtest/gtest.h' file not found [clang-diagnostic-error]
#include "gtest/gtest.h"
         ^| using namespace clang; | ||
| 
               | 
          ||
| TEST(ScopeReflectionTest, DemangleSymbol) { | ||
| if (llvm::sys::RunningOnValgrind()) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "llvm::sys::RunningOnValgrind" is directly included [misc-include-cleaner]
unittests/CppInterOp/ScopeReflectionTest.cpp:14:
+ #include <llvm/Support/Valgrind.h>| if (llvm::sys::RunningOnValgrind()) | ||
| GTEST_SKIP() << "XFAIL due to Valgrind report"; | ||
| 
               | 
          ||
| std::string code = R"( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "std::string" is directly included [misc-include-cleaner]
unittests/CppInterOp/ScopeReflectionTest.cpp:14:
+ #include <string>| 
               | 
          ||
| std::string mangled_add_int; | ||
| std::string mangled_add_double; | ||
| compat::maybeMangleDeclName(Add_int, mangled_add_int); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "compat::maybeMangleDeclName" is directly included [misc-include-cleaner]
unittests/CppInterOp/ScopeReflectionTest.cpp:3:
- #include "clang-c/CXCppInterOp.h"
+ #include "/github/workspace/lib/Interpreter/Compatibility.h"
+ #include "clang-c/CXCppInterOp.h"There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm. Please update the pr and the commit messages accordingly before merging.
e6ff45b    to
    67fef53      
    Compare
  
    DemangleSymbol and IsClassPolymorphic APIsDemangle and IsClassPolymorphic APIs
      67fef53    to
    a021250      
    Compare
  
    
Description
Implements
DemangleSymbolandIsClassPolymorphicAPIs. These are needed to implementCppyy::GetActualClass.Fixes
Fixes 8 tests on cppyy, once
Cppyy::GetActualClassis implemented.Testing
Implemented necessary tests.
Checklist